Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to spdlog>=1.11.0, fmt>=9.1.0. #1177

Merged
merged 20 commits into from
Feb 13, 2023

Conversation

bdice
Copy link
Contributor

@bdice bdice commented Dec 7, 2022

Description

Updates to spdlog>=1.11.0 and fmt>=9.1.0. Also resolves some issues with spdlog in the librmm conda packages. Thanks @robertmaynard for helping advise me on this PR.

We need to test this downstream before merging. Perhaps with cuML or some other library.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@github-actions github-actions bot added the conda label Dec 7, 2022
@bdice bdice added non-breaking Non-breaking change 5 - DO NOT MERGE Hold off on merging; see PR for details improvement Improvement / enhancement to an existing function labels Dec 7, 2022
@bdice bdice self-assigned this Dec 7, 2022
@bdice bdice marked this pull request as ready for review December 7, 2022 20:54
@bdice bdice requested a review from a team as a code owner December 7, 2022 20:54
@codecov-commenter
Copy link

codecov-commenter commented Dec 7, 2022

Codecov Report

Base: 0.00% // Head: 0.00% // No change to project coverage 👍

Coverage data is based on head (62c1bf1) compared to base (f733e6f).
Patch has no changes to coverable lines.

Additional details and impacted files
@@             Coverage Diff              @@
##           branch-23.02   #1177   +/-   ##
============================================
  Coverage          0.00%   0.00%           
============================================
  Files                 6       6           
  Lines               421     421           
============================================
  Misses              421     421           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

dependencies.yaml Outdated Show resolved Hide resolved
dependencies.yaml Outdated Show resolved Hide resolved
conda/recipes/librmm/meta.yaml Show resolved Hide resolved
conda/recipes/librmm/meta.yaml Outdated Show resolved Hide resolved
@kkraus14
Copy link
Contributor

@bdice gentle ping on this. Anything I can do to help move this forward?

@ajschmidt8
Copy link
Member

gentle ping on this. Anything I can do to help move this forward?

@kkraus14, for awareness, Bradley is OoO until 1/17.

@bdice
Copy link
Contributor Author

bdice commented Jan 12, 2023

This is on my radar. I’ve been working on other CI migration and platform support issues since the holidays but will return to it as soon as I can.

@kkraus14
Copy link
Contributor

Just want to make sure this doesn't slip to 23.04 as this is a growing blocker as the conda-forge pinnings are at spdlog 1.11.0 and fmt 9, which means all new packages that rely on either of these libraries at runtime won't be able to be used in an environment with RAPIDS libraries.

I'm happy to help drive this to completion if I can help in any way.

@bdice
Copy link
Contributor Author

bdice commented Jan 19, 2023

@kkraus14 Thanks, I didn’t fully recognize the severity. I’ll prioritize this first thing tomorrow.

@robertmaynard
Copy link
Contributor

@kkraus14 Thanks, I didn’t fully recognize the severity. I’ll prioritize this first thing tomorrow.

To help keep conda and non conda builds in sync I have opened a rapids-cmake PR ( rapidsai/rapids-cmake#342 ) to update the spdlog version when not using conda.

@bdice
Copy link
Contributor Author

bdice commented Jan 19, 2023

@kkraus14 This is ready for additional review. I'll run some tests with downstream packages to ensure cuml and others can build with these changes.

(edit: looks like you approved as I was writing this comment!)

Copy link
Member

@ajschmidt8 ajschmidt8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving ops-codeowner file changes

@kkraus14 kkraus14 mentioned this pull request Feb 6, 2023
7 tasks
@kkraus14
Copy link
Contributor

kkraus14 commented Feb 6, 2023

@kkraus14 I’m leaning towards bringing in fmt explicitly in rapids-cmake. The different bundling approaches in conda packages and CPM fetches are not ideal. Thoughts @robertmaynard?

Opened a PR (rapidsai/rapids-cmake#364) regardless of the short term decision here. My plan is once that lands then RMM can be moved to using fmt explicitly instead of via the spdlog headers.

One thing that I'm not clear on is that the spdlog conda packages currently use the SPDLOG_FMT_EXTERNAL option of spdlog (https://github.com/gabime/spdlog/blob/v1.x/CMakeLists.txt#L88) which uses an external fmt library via the fmt::fmt target. There's also an option of SPDLOG_FMT_EXTERNAL_HO (https://github.com/gabime/spdlog/blob/v1.x/CMakeLists.txt#L89) which then uses a header only fmt via the fmt::fmt-header-only target. I presume the SPDLOG_FMT_EXTERNAL_HO option makes more sense for us in the context where we're fetching spdlog as opposed to finding it?

@bdice
Copy link
Contributor Author

bdice commented Feb 6, 2023

@kkraus14 Yes, that sounds fine: conda packages can use the fmt library (dependency of spdlog) and CPM fetches of spdlog can rely on a header-only fmt that is also fetched by rapids-cmake.

@kkraus14
Copy link
Contributor

kkraus14 commented Feb 6, 2023

@kkraus14 Yes, that sounds fine: conda packages can use the fmt library (dependency of spdlog) and CPM fetches of spdlog can rely on a header-only fmt that is also fetched by rapids-cmake.

Great, I'll tackle that in a follow up PR to rapidsai/rapids-cmake#364 once we bump the spdlog version to 1.11.0 since we'll need to change the rapids_cpm_spdlog logic a bit to find/fetch fmt first.

@bdice bdice changed the base branch from branch-23.02 to branch-23.04 February 8, 2023 18:57
rapids-bot bot pushed a commit to rapidsai/rapids-cmake that referenced this pull request Feb 8, 2023
Adds fmt 9.1.0 to rapids-cmake via `rapids_cpm_fmt` based on discussion in rapidsai/rmm#1177. Nothing should be using `rapids_cpm_fmt` yet so we don't need to version align it with spdlog until spdlog is updated to `1.11.0`.

Depends on #366

Authors:
  - Keith Kraus (https://github.com/kkraus14)
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - Bradley Dice (https://github.com/bdice)

URL: #364
@kkraus14
Copy link
Contributor

kkraus14 commented Feb 9, 2023

Next rapids-cmake PR that bumps spdlog to 1.11.0 and handles the fmt dependency is here: rapidsai/rapids-cmake#368

@kkraus14
Copy link
Contributor

@bdice the rapids-cmake PR should be good to go where this PR should probably be updated to use fmt explicitly instead of indirectly via spdlog. I'm happy to make these changes if you'd like, but I'd need you to give me the ability to write to your branch (since it seems I'm no longer considered a maintainer of this repo which is a bit disappointing but a separate issue to be discussed).

@kkraus14 kkraus14 requested a review from a team as a code owner February 10, 2023 22:41
@github-actions github-actions bot added the CMake label Feb 10, 2023
@github-actions github-actions bot added the Python Related to RMM Python API label Feb 10, 2023
@rapids-bot rapids-bot bot merged commit db92493 into rapidsai:branch-23.04 Feb 13, 2023
rapids-bot bot pushed a commit that referenced this pull request Feb 13, 2023
PR #1177 was merged a little too early when CI passed due to the presence of a `/merge` comment and sufficient approvals. This reverts a temporary change to the rapids-cmake repo that is no longer needed because rapidsai/rapids-cmake#368 has been merged.

Authors:
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - Robert Maynard (https://github.com/robertmaynard)

URL: #1209
ajschmidt8 added a commit to ajschmidt8/integration that referenced this pull request Feb 21, 2023
This PR adds the `fmt` conda package to `rapids-build-env`.

This change is similar to the changes introduced in rapidsai/rmm#1177 and should help resolve the `devel` image build errors that have been occurring lately (e.g. [these](https://gpuci.gpuopenanalytics.com/job/rapidsai/job/docker/job/rapidsai-core-devel/1888/BUILD_IMAGE=rapidsai%2Frapidsai-core-dev-nightly,CUDA_VER=11.8,FROM_IMAGE=gpuci%2Frapidsai,IMAGE_TYPE=devel,LINUX_VER=ubuntu22.04,PYTHON_VER=3.10,RAPIDS_VER=23.04/console)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake conda cpp Pertains to C++ code improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Related to RMM Python API
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants